Skip to content

Fix conftest import #9715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
tapastro merged 5 commits into from
Aug 1, 2025
Merged

Fix conftest import #9715

tapastro merged 5 commits into from
Aug 1, 2025

Conversation

@melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Jul 31, 2025 •
edited
Loading

Closes #9714

Reorganize test data to avoid importing directly from conftest files.

The biggest change here is to the extract_1d conftest file. Currently, only a couple of the fixtures there have been converted to functions to allow them to be reused elsewhere. Since many of them are useful synthetic structures for testing a wide variety of spectral modes, I went ahead and converted almost all the fixtures to functions and moved their implementation to the helpers module. I left a few as fixture-only input data, for tests specific to extract_1d utilities. They could be moved to the helpers later, if they turn out to be useful elsewhere.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

@melanieclarke melanieclarke added this to the Build 12.1 milestone Jul 31, 2025
@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Jul 31, 2025 •
edited
Loading

Regtests shouldn't be needed, but running them anyway just in case:
https://github.com/spacetelescope/RegressionTests/actions/runs/16652526399

All passing.

@codecov
Copy link

codecov bot commented Jul 31, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.11%. Comparing base (3c133e8) to head (35d4022).
⚠️ Report is 268 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #9715   +/-   ##
=======================================
  Coverage   82.11%   82.11%           
=======================================
  Files         367      368    +1     
  Lines       37482    37484    +2     
=======================================
+ Hits        30778    30780    +2     
  Misses       6704     6704

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melanieclarke melanieclarke marked this pull request as ready for review July 31, 2025 15:25
@melanieclarke melanieclarke requested a review from a team as a code owner July 31, 2025 15:25
@melanieclarke melanieclarke requested review from emolter and pllim July 31, 2025 15:25
pllim
pllim reviewed Jul 31, 2025
View reviewed changes
pllim
pllim reviewed Jul 31, 2025
View reviewed changes
Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, just some minor comments. Thanks!

I wonder if we should add __all__ to test helper modules...

@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Jul 31, 2025

I wonder if we should add __all__ to test helper modules...

Probably. I'll add it.

pllim
pllim approved these changes Jul 31, 2025
View reviewed changes
Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

emolter
emolter reviewed Jul 31, 2025
View reviewed changes
Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Melanie, looks great overall. I had a question about some docstring changes

@melanieclarke melanieclarke requested a review from tapastro August 1, 2025 12:37
@tapastro tapastro merged commit b2c8fe9 into spacetelescope:main Aug 1, 2025
32 checks passed
@melanieclarke melanieclarke deleted the fix_conftest_import branch December 1, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes

@tapastro tapastro tapastro approved these changes

@emolter emolter emolter approved these changes

Assignees

No one assigned

Labels

ami exp_to_source extract_1d no-changelog-entry-needed pipeline spec3 pipeline testing

Projects

None yet

Milestone

Build 12.1

Development

Successfully merging this pull request may close these issues.

TST: Do not import directly from conftest

4 participants

@melanieclarke @pllim @tapastro @emolter